Replace locals in debuginfo records during ref_prop and dest_prop #147525

dianqk · 2025-10-09T14:08:15Z

r? cjgillot

rustbot · 2025-10-09T14:08:19Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

cjgillot

I'm not certain this is the best approach. Maintaining this invariant during optimizations will be error-prone. Should SimplifyLocals drop dbg statements whose LHS is dead?

View changes since this review

cjgillot · 2025-10-09T14:18:25Z

compiler/rustc_mir_transform/src/ref_prop.rs

+        location: Location,
+    ) {
+        if let StmtDebugInfo::AssignRef(local, _) = stmt_debuginfo
+            && let Value::Pointer(target, _) = self.targets[*local]


If this mirrors visit_var_debug_info, we need an extra condition on target.projection, don't we?

I think var_debug_info will handle projection. We just need to adjust to the new load.

cjgillot · 2025-10-09T14:19:42Z

tests/mir-opt/debuginfo/ref_prop.remap_debuginfo_locals.ReferencePropagation.diff

 -         StorageDead(_3);
 -         StorageDead(_1);
-+         // DBG: _1 = &(*_5);
+         // DBG: _5 = &(*_5);


Should we keep this statement, or drop it as a tautology?

Hmm, I'm not sure. It looks like LLVM doesn't care about what the value is: https://rust.godbolt.org/z/vc3W5fKr4.

durin42 · 2025-10-09T16:50:05Z

This doesn't fix the case we saw internally in netlink-proto. Could you also give it a shot against the non-minimized reproducer in #147485?

durin42 · 2025-10-09T17:25:27Z

This doesn't fix the case we saw internally in netlink-proto. Could you also give it a shot against the non-minimized reproducer in #147485?

Had a few minutes, so I checked: with this PR applied the reproducer in https://github.com/krasimirgg/netlink-proto branch ice still ICEs. :(

tmiasko · 2025-10-10T09:34:43Z

Assuming the invariant proposed here, the root cause in the original reproducer was in destination propagation (note the replacement of _5 in var debug info, while it remains unchanged in statement debug info).

DestinationPropagation diff for protocol::protocol::::handle_response

--- ./netlink_proto.protocol-protocol-{impl#1}-handle_response.3-2-028.DestinationPropagation.before.mir	2025-10-09 19:33:36.091255817 +0200
+++ ./netlink_proto.protocol-protocol-{impl#1}-handle_response.3-2-028.DestinationPropagation.after.mir	2025-10-09 19:33:36.093145504 +0200
@@ -1,3 +1,3 @@
-// MIR for `protocol::protocol::<impl at src/protocol/protocol.rs:63:1: 66:22>::handle_response` before DestinationPropagation
+// MIR for `protocol::protocol::<impl at src/protocol/protocol.rs:63:1: 66:22>::handle_response` after DestinationPropagation
 
 fn protocol::protocol::<impl at src/protocol/protocol.rs:63:1: 66:22>::handle_response(_1: &mut VecDeque<Response<T, M>>, _2: std::collections::hash_map::OccupiedEntry<'_, RequestId, PendingRequest<M>>, _3: NetlinkMessage<T>) -> () {
@@ -72,5 +72,5 @@
         let mut _5: &protocol::protocol::RequestId;
         scope 2 {
-            debug request_id => _5;
+            debug request_id => _41;
             let _6: log::Level;
             let _19: bool;
...
@@ -588,5 +588,5 @@
     bb12: {
         // DBG: _5 = &((*_78).0: protocol::protocol::RequestId);
-        StorageLive(_19);
+        nop;
         _20 = discriminant((_3.1: netlink_packet_core::NetlinkPayload<T>));
         switchInt(move _20) -> [4: bb2, otherwise: bb1];
...
@@ -664,6 +664,6 @@
         _4 = copy _159;
         _41 = &_4;
-        _5 = copy _41;
-        _25 = copy _163;
+        nop;
+        nop;
         goto -> bb8;
     }
...

…als that are not in debuginfo

rustbot · 2025-10-10T10:31:37Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

dianqk · 2025-10-10T10:41:39Z

Yes. The local in the statement debuginfo was not updated along with the var debuginfo. This may also mix different debugging variables, then mislead debuginfo.

dianqk · 2025-10-10T10:53:42Z

compiler/rustc_middle/src/mir/visit.rs

            }
        }

+        for var_debug_info in &$($mutability)? $body.var_debug_info {


I'm not sure if relying on the order of visiting is reasonable.

dianqk · 2025-10-10T10:57:55Z

This doesn't fix the case we saw internally in netlink-proto. Could you also give it a shot against the non-minimized reproducer in #147485?

Had a few minutes, so I checked: with this PR applied the reproducer in https://github.com/krasimirgg/netlink-proto branch ice still ICEs. :(

🥲 Fixed.

cjgillot · 2025-10-10T13:29:09Z

tests/mir-opt/debuginfo/dest_prop.rs

+
+// EMIT_MIR dest_prop.remap_debuginfo_locals.DestinationPropagation.diff
+#[custom_mir(dialect = "runtime", phase = "post-cleanup")]
+pub fn remap_debuginfo_locals(a: bool, b: &bool) -> &bool {


Could you add some filecheck annotations?

Oops, added.

dianqk · 2025-10-10T14:10:29Z

I'm not certain this is the best approach. Maintaining this invariant during optimizations will be error-prone. Should SimplifyLocals drop dbg statements whose LHS is dead?

View changes since this review

I think dbg statements should be dropped only when var_debug_info is dropped. StripDebugInfo and inlining does it.

cjgillot · 2025-10-10T14:32:01Z

I think dbg statements should be dropped only when var_debug_info is dropped. StripDebugInfo and inlining does it.

TBH, I'm a bit worried by the difficulty to maintain the invariant. For instance if GVN or SingleUseConsts starts replacing some places by constants in var_debug_info.

But already, r=me with the tests and the changes to ref_prop and dest_prop.

cjgillot · 2025-10-10T17:46:17Z

@bors r+

bors · 2025-10-10T17:46:19Z

📌 Commit 1ee2c58 has been approved by cjgillot

It is now in the queue for this repository.

Replace locals in debuginfo records during ref_prop and dest_prop Fixes rust-lang#147485. r? cjgillot

Rollup of 12 pull requests Successful merges: - #145651 (Regression test for const promotion with Option<Ordering>) - #145722 (implement Extend<{Group, Literal, Punct, Ident}> for TokenStream) - #146520 (Promote armv8r-none-eabihf target to Tier 2) - #146522 (Promote armv7a-none-eabihf to Tier 2) - #147289 (Mitigate `thread_local!` shadowing issues) - #147515 (Update rustc-perf submodule) - #147522 (compiletest: Use the same directive lines for EarlyProps and ignore/only/needs) - #147525 (Replace locals in debuginfo records during ref_prop and dest_prop) - #147544 (Remove StatementKind::Deinit.) - #147551 (remove `#[rustc_inherit_overflow_checks]` from `is_multiple_of`) - #147553 (Move `wasm32-wasip3` to the tier 3 table) - #147562 (Stabilize `NonZero<u*>::div_ceil`) r? `@ghost` `@rustbot` modify labels: rollup

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 9, 2025

rustbot assigned cjgillot Oct 9, 2025

cjgillot reviewed Oct 9, 2025

View reviewed changes

dianqk added 3 commits October 10, 2025 18:30

Add regression test for 147485

690071c

MIR validation: ensure that debuginfo records are not emitted for loc…

9462e73

…als that are not in debuginfo

Replace locals in debuginfo records during ref_prop

b2e81b0

dianqk force-pushed the ref_prop_debuginfo branch from 0c0cf47 to 400c56e Compare October 10, 2025 10:31

dianqk changed the title ~~Replace locals in debuginfo records during ref_prop~~ Replace locals in debuginfo records during ref_prop and dest_prop Oct 10, 2025

dianqk commented Oct 10, 2025

View reviewed changes

cjgillot reviewed Oct 10, 2025

View reviewed changes

Replace locals in debuginfo records during dest_prop

1ee2c58

dianqk force-pushed the ref_prop_debuginfo branch from 400c56e to 1ee2c58 Compare October 10, 2025 14:06

dianqk added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Oct 10, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 10, 2025

dianqk added a commit to dianqk/rust that referenced this pull request Oct 10, 2025

Rollup merge of rust-lang#147525 - dianqk:ref_prop_debuginfo, r=cjgillot

fce1bea

Replace locals in debuginfo records during ref_prop and dest_prop Fixes rust-lang#147485. r? cjgillot

dianqk mentioned this pull request Oct 10, 2025

Rollup of 12 pull requests #147574

Open

Replace locals in debuginfo records during ref_prop and dest_prop #147525

Are you sure you want to change the base?

Replace locals in debuginfo records during ref_prop and dest_prop #147525

Conversation

dianqk commented Oct 9, 2025

Uh oh!

rustbot commented Oct 9, 2025

Uh oh!

cjgillot left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjgillot Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

dianqk Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

cjgillot Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

dianqk Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

durin42 commented Oct 9, 2025

Uh oh!

durin42 commented Oct 9, 2025

Uh oh!

tmiasko commented Oct 10, 2025

Uh oh!

rustbot commented Oct 10, 2025

Uh oh!

dianqk commented Oct 10, 2025

Uh oh!

dianqk Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

dianqk commented Oct 10, 2025

Uh oh!

cjgillot Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

dianqk Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

dianqk commented Oct 10, 2025

Uh oh!

cjgillot commented Oct 10, 2025

Uh oh!

cjgillot commented Oct 10, 2025

Uh oh!

bors commented Oct 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cjgillot left a comment •

edited by rustbot

Loading